-
Notifications
You must be signed in to change notification settings - Fork 13.9k
coercions reviews/misc whatevers #147565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
coercions reviews/misc whatevers #147565
Conversation
This comment has been minimized.
This comment has been minimized.
70c3cf5 to
0d38d86
Compare
This comment has been minimized.
This comment has been minimized.
0d38d86 to
1375123
Compare
This comment has been minimized.
This comment has been minimized.
1375123 to
0b58522
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bce0933 to
eb6434f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e5524ca to
d8775a4
Compare
This comment has been minimized.
This comment has been minimized.
d8775a4 to
612a766
Compare
This comment has been minimized.
This comment has been minimized.
612a766 to
1184da6
Compare
This comment has been minimized.
This comment has been minimized.
797201d to
3ecd1a8
Compare
This comment has been minimized.
This comment has been minimized.
3ecd1a8 to
f23f16b
Compare
6179239 to
2958ef0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (08e75d5): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.4%, secondary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.6%, secondary 3.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 473.971s -> 475.243s (0.27%) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
coercions reviews/misc whatevers
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (460970f): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.4%, secondary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 5.6%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.716s -> 473.379s (-0.49%) |
aeec7db to
1fa1cc2
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
coercions reviews/misc whatevers
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (29354ec): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.8%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.797s -> 477.705s (0.19%) |
"remove normalize call"
Fixes #132765 in theory breaking as we might now treat some previously-ambig hr aliases as rigid when we shouldn't. we may want to explicitly normalize the coercion target in
fcx.coercelike the new solver does"leak check and lub for closure<->closure coerce-lubs"
Fixes rust-lang/trait-system-refactor-initiative#233
the
|x| x + 1expr has a type ofClosure(?31t)which we wind up inferring the RPIT to. TheCoerceManyret_coercionfor the wholepeculiartypeck has an expected type ofRPIT(unnormalized). When we type check thereturn |x| x + 1expr we go from the never type toClosure(?31t)which then participates in theret_coerciongiving us acoerce-lub(RPIT, Closure(?31t)).Normalizing
RPITgives us someClosure(?50t)where?31tand?50thave been unified with?31tas the root var.resolve_vars_if_possibledoesn't resolve infer vars to their roots so these wind up with different structural identities so the fast path doesn't apply and we fall back to coercing to afnptr. cc #147193 which also fixes thisNew solver probably just gets more inference variables here because canonicalization + generally different approach to normalization of opaques. Idk :3
This technically allows more code to compile (see the test using TAIT in the commit). I don't think this can be observed on stable though.
In theory this could break existing code that relied on coercing a closure to itself resulting in a fnptr. I don't expect this to really happen in practice and this is an "obvious" bug fix.
misc additional leak checks :)
We now leak check in a lot more places:
let a = b;will coerceb->abut if there's no coercion it just falls back to subtyping which is now leak checked.closure->fnptrnow leak checksfndef<->fndefnow leak checksfndef<->closureorclosure<->fndefnow leak checksFor 1 and 2 this should in theory only affect dead code as inside of livecode the borrow checker will wind up checking these constraints anyway. For 2 specifically it's worth noting that we already leak check
fndef->fnptrandfnptr->fnptrcoercions on stable.3 allows more code to compile. Previously by not leak checking we could wind up thinking two fndefs were equal and so avoid coercing to a fnptr. Then borrowck would wind up emitting an error due to incompatible binders. See
tests/ui/coercion/leak_check_fndef_lub.rswhich failed to compile before this PR.3 and 4 both stop some code from compiling in cases where the coerce-lub can only succeed due to
lubarbitrarily returning thelhsfor binders instead of a true "lub". See theorder_dependence_xtests intests/ui/coercion/leak_check_lub_to_fnptr.rs. Note that forfnptr<->fnptrcoerce-lubs we already leak check on stable preventing equivalent code from compilingsimilar to 1/2, both 3 and 4 should in theory also cause some code to stop compiling as we will be leak checking in dead code where borrowck is unable to emit errors.
TODO: I need to add some more tests here and properly link to them I think
"account for safe target features in fndef<->closure and fndef<->fndef coerce-lubs"
We previously did not take safe target features into account when creating the fn sig for fndefs during
fndef<->closureorfndef<->fndefcoerce-lubs. We now do allowingcoerce-lubto produce a safe fn pointer when fndefs with safe target features are involved. This is consistent with existing (non lub) coercion logic forfndef->fnptrwhich allows coercing to a safe fn pointer.r? ghost